fix: ignore unknown resolved warning IDs - #67

Walkthrough

The briefing service now supplies an allowlist of active warning IDs to the LLM, preserves it during retries, and filters unknown resolved IDs after summarization instead of retrying. Prompts, tests, and warning lifecycle documentation describe the updated contract and behavior.

Changes

Resolved warning handling

Estimated code review effort: 3 (Moderate) | ~20 minutes

❌ Failed checks (1 warning)

• X
• Mastodon
• Reddit
• LinkedIn

Description

The PR introduces a non-obvious architectural decision: exposing allowed_resolved_warning_ids to
the LLM and deterministically filtering unknown resolved_warning_ids before persistence. This
decision is not documented in docs/notes.md with rationale/trade-offs/boundaries as required.

Code

weather_briefing/service.py[R412-423]

+        unknown_resolved_warning_ids = set(result.resolved_warning_ids) - active_warning_ids
+        if unknown_resolved_warning_ids:
+            _LOGGER.warning(
+                "Ignoring %d resolved warning ID(s) that are not currently active",
+                len(unknown_resolved_warning_ids),
+            )
+            result = replace(
+                result,
+                resolved_warning_ids=tuple(
+                    warning_id for warning_id in result.resolved_warning_ids if warning_id in active_warning_ids
+                ),
+            )

Relevance

⭐⭐⭐ High

Team has added rationale/boundaries to docs/notes.md for similar non-obvious workflow changes
(PR#54, PR#63).

PR-#54
PR-#63

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 2141673 requires documenting non-obvious architectural decisions in
docs/notes.md. The changed service logic introduces a new contract boundary (allowlisted resolved
IDs) and a deterministic filtering behavior, but docs/notes.md contains no corresponding decision
entry for this change.

Rule 2141673: Document non-obvious architectural decisions in docs/notes.md
weather_briefing/service.py[412-423]
docs/notes.md[1-3]
docs/design.md[93-95]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
This PR changes warning-resolution behavior by introducing an allowlist (`allowed_resolved_warning_ids`) and ignoring unknown `resolved_warning_ids` before persistence, but the non-obvious decision (rationale, trade-offs, operating boundaries) is not documented in `docs/notes.md`.

## Issue Context
The code now filters unknown IDs and logs only the ignored count, which affects reliability (avoids contract-repair retries) and privacy/observability (do not log model-generated IDs).

## Fix Focus Areas
- docs/notes.md[1-120]
- weather_briefing/service.py[412-423]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

After filtering out unknown resolved warning IDs, the service updates only
BriefingResult.resolved_warning_ids but leaves BriefingResult.raw_payload containing the original
(unfiltered) resolved_warning_ids. This makes raw_payload inconsistent with the persisted/effective
result and unsafe to treat as the canonical post-normalization representation.

Code

weather_briefing/service.py[R412-423]

+        unknown_resolved_warning_ids = set(result.resolved_warning_ids) - active_warning_ids
+        if unknown_resolved_warning_ids:
+            _LOGGER.warning(
+                "Ignoring %d distinct resolved warning ID(s) that are not currently active",
+                len(unknown_resolved_warning_ids),
+            )
+            result = replace(
+                result,
+                resolved_warning_ids=tuple(
+                    warning_id for warning_id in result.resolved_warning_ids if warning_id in active_warning_ids
+                ),
+            )

Relevance

⭐⭐ Medium

No prior reviews mention keeping raw_payload synced; team does value boundary consistency (PR33,
PR45).

PR-#33
PR-#45

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The service filters only the dataclass field, while parse_result stores the original structured
model output (including the original resolved_warning_ids) into raw_payload, so the two will diverge
after filtering.

weather_briefing/service.py[366-423]
weather_briefing/llm.py[283-331]
weather_briefing/models.py[205-218]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`BriefingService._run()` filters unknown `resolved_warning_ids` using `dataclasses.replace`, but only updates the typed `resolved_warning_ids` field. `BriefingResult.raw_payload` (which is populated from the original structured LLM response) remains unchanged and can therefore disagree with `result.resolved_warning_ids` after normalization.

## Issue Context
`raw_payload` is part of `BriefingResult` and currently represents the exact parsed model output. Once the service applies post-processing, the object contains two conflicting representations of `resolved_warning_ids`.

## Fix Focus Areas
- weather_briefing/service.py[411-423]
- weather_briefing/llm.py[283-331]
- weather_briefing/models.py[205-218]

## Suggested fix
When filtering unknown IDs, also update `raw_payload["resolved_warning_ids"]` to match the filtered tuple (or explicitly document/rename `raw_payload` to indicate it is intentionally unnormalized and must not be used as a post-processed representation).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Description

The service logs the number of unknown resolved warning IDs using a set difference, which counts
distinct unknown IDs rather than the number of unknown entries returned by the model. If the model
repeats an unknown ID, the warning message’s count will not match how many list entries were
actually discarded, which can skew monitoring depending on intended semantics.

Code

weather_briefing/service.py[R412-417]

+        unknown_resolved_warning_ids = set(result.resolved_warning_ids) - active_warning_ids
+        if unknown_resolved_warning_ids:
+            _LOGGER.warning(
+                "Ignoring %d resolved warning ID(s) that are not currently active",
+                len(unknown_resolved_warning_ids),
+            )

Relevance

⭐⭐⭐ High

Similar “count semantics must match reality” observability fixes were merged (RSS attempt counts
PR#48; alert semantics PR#56).

PR-#48
PR-#56

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The log count is derived from a set difference, which inherently deduplicates repeated IDs;
meanwhile the response schema and domain model allow duplicates because resolved_warning_ids is a
plain list/tuple of strings with no uniqueness constraint.

weather_briefing/service.py[411-423]
weather_briefing/llm.py[102-113]
weather_briefing/models.py[205-218]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`weather_briefing.service` computes `unknown_resolved_warning_ids` via `set(result.resolved_warning_ids) - active_warning_ids`, then logs `len(unknown_resolved_warning_ids)`. This counts *distinct* unknown IDs, not the number of unknown entries produced by the model, so duplicates can make the logged count smaller than the number of discarded items.

## Issue Context
- `resolved_warning_ids` is modeled as a list/tuple of strings with no uniqueness constraint.
- The PR goal is to avoid leaking IDs; counting entries can be done without logging the values.

## Fix Focus Areas
- weather_briefing/service.py[411-423]

### Suggested approach
- Compute unknown entries without deduplication:
 - `unknown_entries = [wid for wid in result.resolved_warning_ids if wid not in active_warning_ids]`
 - If `unknown_entries`: log `len(unknown_entries)`.
 - Filter result as already done.
- (Optional) Add/adjust a unit test to cover duplicate unknown IDs if you want to lock in “entry count” semantics.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

• Expose currently active warning IDs as the only allowed resolved_warning_ids values.
• Filter unknown resolved IDs before persistence to avoid contract-repair retries and delivery
 failures.
• Add regression tests and document the degraded-but-observable behavior boundary.

graph TD
  db[("State store")] --> aw["Active warnings"]
  aw --> svc["BriefingService"]
  svc --> payload["Payload (allowed IDs)"] --> prompt["SYSTEM_PROMPT"] --> llm{{"LLM provider"}} --> res["BriefingResult"]
  res --> filt["Filter unknown resolves"] --> db
  subgraph Legend
    direction LR
    _db[("Database")] ~~~ _svc["Service/Module"] ~~~ _ext{{"External"}}
  end

Loading

The following are alternative approaches to this PR:

1. Keep strict validation + stronger repair prompt

• ➕ Preserves a single clear contract: any invalid resolved ID fails fast.
• ➕ Forces the model to conform, potentially catching upstream prompt/model regressions earlier.
• ➖ Still risks exhausting LLM_MAX_ATTEMPTS on no-op, non-user-visible state mutations.
• ➖ Continues to block delivery due to an identifier that cannot be actioned anyway.

2. Deterministic resolution mapping (no IDs from LLM)

• ➕ Removes ID handling from the model entirely (more robust, less prompt-dependent).
• ➕ Allows richer matching (e.g., resolve by title/status/provider key) while keeping state consistent.
• ➖ Requires additional domain logic/provider integration and potentially schema changes.
• ➖ Higher implementation complexity and greater risk of mismatches without careful design.

Recommendation: The chosen approach (allowlist + pre-persistence filtering with count-only logging) is the best fit given resolved_warning_ids is a bookkeeping field with no direct user-visible effect. It prevents delivery failures and eliminates wasted contract-repair retries while preserving correct behavior for valid, active IDs. If resolved IDs later influence rendering or external side effects, revisit and move toward deterministic provider/app-owned warning identity (or reinstate strict failures).

Bug fix (2) +18 / -8

prompts.pyConstrain 'resolved_warning_ids' to copy from input allowlist +2/-1

Constrain 'resolved_warning_ids' to copy from input allowlist

• Updates the system prompt so each 'resolved_warning_ids' entry must be copied verbatim from 'input.allowed_resolved_warning_ids', and prohibits inventing IDs when no match exists.

weather_briefing/prompts.py

service.pyPass allowed resolved IDs to LLM and filter unknown IDs before persistence +16/-7

Pass allowed resolved IDs to LLM and filter unknown IDs before persistence

• Adds 'allowed_resolved_warning_ids' to the LLM payload based on currently active warnings, and propagates the same allowlist into contract-repair payloads. Removes strict validation failure on unknown resolved IDs and instead logs the distinct unknown count and persists only the intersection with active IDs via an immutable 'BriefingResult' replace.

weather_briefing/service.py

Tests (1) +9 / -4

test_service.pyChange contract-retry test to assert unknown resolved IDs are ignored +9/-4

Change contract-retry test to assert unknown resolved IDs are ignored

• Reworks the regression test to ensure allowed IDs are provided to the LLM payload and that unknown resolved IDs are filtered without triggering retry attempts. Asserts a warning log message reports the distinct unknown count without leaking IDs.

tests/test_service.py

Documentation (3) +4 / -2

design.mdDocument allowlisted warning resolutions and filtering behavior +1/-1

Document allowlisted warning resolutions and filtering behavior

• Updates the warning-memory design to state that the LLM receives an explicit allowlist of resolvable warning IDs. Documents that unknown 'resolved_warning_ids' are filtered before persistence and only a distinct-count is logged.

docs/design.md

notes.mdExplain degraded handling rationale for unknown resolved warning IDs +2/-0

Explain degraded handling rationale for unknown resolved warning IDs

• Adds an explicit design note that warning resolution is a degradable field: unknown IDs are filtered rather than triggering contract repair. Records the observability and re-evaluation criteria for returning to strict failure semantics.

docs/notes.md

requirements.mdClarify requirements: only active warning IDs can be resolved; unknown IDs must not block delivery +1/-1

Clarify requirements: only active warning IDs can be resolved; unknown IDs must not block delivery

• Tightens the functional requirement so the LLM may only resolve warnings using currently active IDs. Specifies that unknown resolution IDs must not change state and must not block summarization/delivery.

docs/requirements.md